[refactor][java] Improve docs and code quality about KeyValueSchema usages - #17256
Conversation
BewareMyPower
added
component/client-java
type/refactor
|
@BewareMyPower Please provide a correct documentation label for your PR. |
…sages ### Motivation First, it's hard to know the default encoding type of a KeyValue schema from the API and its JavaDocs, see https://github.com/apache/pulsar/blob/fd9489771959f3e722656e4b70d4bd891a13f690/pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Schema.java#L386-L391 The default encoding type could only be found in `KeyValueSchemaImpl#of` method from another module (`pulsar-client`). Second, there are much raw use of parameterized class, including `KeyValueSchema` and `KeyValue` from `pulsar-common` module. This use harms the code quality because it could lose the type safety. ### Modifications For the 1st issue, call another overload of `Schema#KeyValue` method and note the default encoding type so that it's clear that the encoding type of `Schema#KeyValue(Schema<K>, Schema<V>)` method is `INLINE`. For the 2nd issue, leverage Java's type inference feature to make sure the type arguments of `KeyValue` are the same as the type arguments of `KeyValueSchema` in a `TypedMessageBuilderImpl` instane. `getKeyValueSchema` method is added to simplify the code. ### Verifying this change - [ ] Make sure that the change passes the CI checks. This change is already covered by existing tests, such as `SchemaTest` and `NullValueTest` in `pulsar-broker` module.
1ab46fa to
9058334
Compare
|
@Demogorgon314 @codelipenghui @eolivelli @lhotari @gaoran10 Could you take a look? |
| msgMetadata.setNullPartitionKey(true); | ||
| return this; | ||
| } | ||
| getKeyValueSchema().ifPresent(keyValueSchema -> checkArgument( |
There was a problem hiding this comment.
While you're here, can you explain why when the schema is KeyValue, and the encoding is SEPARATED, it is forbidden to use this method to se the key of the message?
There was a problem hiding this comment.
Because when the encoding is SEPARATED, the partition key should be set by a KeyValue object.
Producer<KeyValue<String, String>> producer = client.newProducer(
Schema.KeyValue(Schema.STRING, Schema.STRING, KeyValueEncodingType.SEPARATED))
.topic("my-topic")
.create();
producer.newMessage().value(new KeyValue<>("key", "value")).send()With the SEPARATED key value encoding type, the code above will send a message whose partition key is "key" and value is "value". If it's allowed to set the partition key here, conflicts will happen.
There is no design of the key value schema, I just guessed the reason why key and keyBytes methods are forbidden and followed the previous behavior in this PR
There was a problem hiding this comment.
I don't understand why.
Why can't we do
.newMessage() // this creates a builder
.key(theKey)
.value(theValue)
.send()There was a problem hiding this comment.
Because the schema type is Schema<KeyValue<K, V>>, the value's type must be KeyValue<K, V>, then the code will look like:
.key("key")
.value(new KeyValue<>("key-1", "value"))The root cause is that Pulsar only stores the schema of the value. So it can only support key schema by passing a pair of "key" and "value" as the whole value.
|
The pr had no activity for 30 days, mark with Stale label. |
…sages (apache#17256) (cherry picked from commit 5d6a88e) (cherry picked from commit a8b00fd)
…sages (apache#17256) (cherry picked from commit 5d6a88e) (cherry picked from commit a8b00fd)
8 participants
Motivation
First, it's hard to know the default encoding type of a KeyValue schema
from the API and its JavaDocs, see
pulsar/pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Schema.java
Lines 386 to 391 in fd94897
The default encoding type could only be found in
KeyValueSchemaImpl#ofmethod from another module (
pulsar-client).Second, there are much raw use of parameterized class, including
KeyValueSchemaandKeyValuefrompulsar-commonmodule. This useharms the code quality because it could lose the type safety.
Modifications
For the 1st issue, call another overload of
Schema#KeyValuemethod andnote the default encoding type so that it's clear that the encoding
type of
Schema#KeyValue(Schema<K>, Schema<V>)method isINLINE.For the 2nd issue, leverage Java's type inference feature to make sure
the type arguments of
KeyValueare the same as the type arguments ofKeyValueSchemain aTypedMessageBuilderImplinstane.getKeyValueSchemamethod is added to simplify the code.Verifying this change
This change is already covered by existing tests, such as
SchemaTestandNullValueTestinpulsar-brokermodule.Documentation
Check the box below or label this PR directly.
Need to update docs?
doc-required(Your PR needs to update docs and you will update later)
doc-not-needed(Please explain why)
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)